Perf drawFramwork #2474

etpinard · 2018-03-13T20:58:24Z

Here are two perf commits in Cartesian.drawFramework that will help splom subplot generation. 🐎

At 400 subplots (or 20 splom dimensions 😏 ), these commits are able to speed up drawFramework from ~1000ms down under 200ms.

Note that it might be nice to make the new joinLayer from 7d881f6 a standard Lib function and replace all our selectAll().data([0]) with it. Waiting on @alexcjohnson before going through with it.

- replace Object.keys + for-loop with for-in - replace slow subplots.indexOf

- to replace expensive querySelectorAll with plain querySelector

alexcjohnson · 2018-03-13T21:35:18Z

Wow, I would not have guessed that those two were such a big drain, impressive find! Yes please, 🔪 all the selectAll(...).data([0])! The name joinLayer has never meant much to me though... I suppose I'd get used to it, but what about something like Lib.ensureSingle?

etpinard · 2018-03-13T21:42:41Z

I suppose I'd get used to it, but what about something like Lib.ensureSingle

I like the sound of ensureSingle. Thanks!

- meant to replace all our `selectAll().data([0])` calls by something DRYer and faster.

- note that one test suite had to be updated, simply because ensureSingle adds class names in a different order than previously.

alexcjohnson · 2018-03-14T19:46:27Z

src/components/colorbar/draw.js

-            .each(function() {
+        var container = Lib.ensureSingle(fullLayout._infolayer, 'g', id, function(s) {
+            s.classed(cn.colorbar, true);
+            s.each(function() {


you don't like chaining here? ie

s.classed(...) .each(...)

Good catch. Thanks!

done in -> 8e6032d

- this appear to be no longer needed - adapt plot_api test to relied on layoutStyles spy counts

alexcjohnson · 2018-03-14T20:06:05Z

test/jasmine/tests/click_test.js

-            expect(node.classList[0]).toBe('drag');
-            expect(node.classList[1]).toBe('nwdrag');
+            expect(node.classList[1]).toBe('drag');
+            expect(node.classList[0]).toBe('nwdrag');


OK as is, but this seems ripe for a custom matcher like:

expect(node).toBeClassed(['drag', 'nwdrag', 'cursor-nw-resize']);

That would be order-independent but also verify these are the only classes applied.

Good call.

cartesian_test.js uses assertions like:

expect(fills[0].classList.contains('js-tozero')).toBe(true)

which could work here too, but yeah checking adding a custom assertion that test multiple class names at once should be worthwhile.

done in b974eab

alexcjohnson · 2018-03-14T20:19:26Z

src/plot_api/plot_api.js

@@ -245,9 +245,7 @@ exports.plot = function(gd, data, layout, config) {
                .attr('height', fullLayout.height);
        }

-        return Lib.syncOrAsync([
-            subroutines.layoutStyles
-        ], gd);


Too funny, so it just works! We may never know what obviated this...

just works^TM

etpinard · 2018-03-14T20:19:29Z

src/plot_api/plot_api.js

-        return Lib.syncOrAsync([
-            subroutines.layoutStyles
-        ], gd);
+        return Plots.previousPromises(gd);


@alexcjohnson tests are ✅ after removing the extra layoutStyles call. Good riddance ⛵

... even though don't look bad to etpinard's 👀

- to easily test all items in class list while not having to worry about class name ordering.

alexcjohnson · 2018-03-14T21:37:37Z

Beautiful! 💃 💃 💃

etpinard · 2018-03-14T21:41:46Z

Ok. This probably deserves to be in the next minor release 1.36.0, so I'll wait a few more days in case we need to make another patch release in 1.35.x series.

... so that we don't have to traverse the DOM whenever we add a clipPath url 🐎

.. so that in the event where window.location changes on pages with a <base>, our clip path urls will be properly updated.

- this can speed up Axes.doTicks by 50ms on 50x50 subplots grids

- so that drawTitle() can reuse it - this can speed up doTicks by 200ms at 50x50 subplots

More optimization for cartesian subplots

etpinard added 2 commits March 13, 2018 16:28

perf makeSubplotData

d305564

- replace Object.keys + for-loop with for-in - replace slow subplots.indexOf

replace selectAll().data([0]) with select() + .size()

7d881f6

- to replace expensive querySelectorAll with plain querySelector

etpinard added status: reviewable labels Mar 13, 2018

etpinard mentioned this pull request Mar 13, 2018

Scatter Plot Matrix (aka SPLOM) discussion #2372

Closed

etpinard added 2 commits March 14, 2018 15:31

add ensureSingle & ensureSingleById lib methods

5cc9af9

- meant to replace all our `selectAll().data([0])` calls by something DRYer and faster.

replace selectAll().data([0]) with ensureSingle(ById)

e32434e

- note that one test suite had to be updated, simply because ensureSingle adds class names in a different order than previously.

alexcjohnson reviewed Mar 14, 2018

View reviewed changes

🔪 layoutStyles call in drawFramework step

e810c1e

- this appear to be no longer needed - adapt plot_api test to relied on layoutStyles spy counts

alexcjohnson reviewed Mar 14, 2018

View reviewed changes

etpinard commented Mar 14, 2018

View reviewed changes

etpinard added 3 commits March 14, 2018 17:04

bring back d3-esque method chains

8e6032d

... even though don't look bad to etpinard's 👀

add toBeClassed custom matcher

b974eab

- to easily test all items in class list while not having to worry about class name ordering.

fixup typo

fc8c386

etpinard added this to the v1.36.0 milestone Mar 14, 2018

stash base url on Drawing module object

5887104

... so that we don't have to traverse the DOM whenever we add a clipPath url 🐎

etpinard force-pushed the perf-drawFramwork branch from e22aac5 to 5887104 Compare March 15, 2018 01:49

etpinard added 5 commits March 16, 2018 11:48

clear stashed base url off Drawing module on replot

42ec6a0

.. so that in the event where window.location changes on pages with a <base>, our clip path urls will be properly updated.

do not try to clear ax layers on first render

928ff6d

- this can speed up Axes.doTicks by 50ms on 50x50 subplots grids

use ensureSingle in plot bg lsInner routine

a2c5c3d

lint in axes.js

49177a2

stash tickLabels selection in doTicks scope

8cf99a6

- so that drawTitle() can reuse it - this can speed up doTicks by 200ms at 50x50 subplots

etpinard mentioned this pull request Mar 20, 2018

More optimization for cartesian subplots #2487

Merged

etpinard added 2 commits March 26, 2018 16:41

fixup typo in select query & remove neccessary branch

5307125

🔪 unnecessary scoped var

4059764

Merge pull request #2487 from plotly/perf-drawFramwork-more

50ab638

More optimization for cartesian subplots

etpinard merged commit 9a79056 into master Mar 26, 2018

etpinard deleted the perf-drawFramwork branch March 26, 2018 21:54

etpinard mentioned this pull request Mar 28, 2018

Introducing splom traces #2505

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perf drawFramwork #2474

Perf drawFramwork #2474

etpinard commented Mar 13, 2018 •

edited

Loading

alexcjohnson commented Mar 13, 2018

etpinard commented Mar 13, 2018

alexcjohnson Mar 14, 2018

etpinard Mar 14, 2018

etpinard Mar 14, 2018

alexcjohnson Mar 14, 2018

etpinard Mar 14, 2018

etpinard Mar 14, 2018

alexcjohnson Mar 14, 2018

etpinard Mar 14, 2018

etpinard Mar 14, 2018

alexcjohnson commented Mar 14, 2018

etpinard commented Mar 14, 2018

Perf drawFramwork #2474

Perf drawFramwork #2474

Conversation

etpinard commented Mar 13, 2018 • edited Loading

alexcjohnson commented Mar 13, 2018

etpinard commented Mar 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented Mar 14, 2018

etpinard commented Mar 14, 2018

etpinard commented Mar 13, 2018 •

edited

Loading